feat: add rewrite manifests#774
Conversation
zhjwpku
force-pushed
the
support_rewrite_manifests
branch
7 times, most recently
from
18bade1 to
7de6949
Compare
zhjwpku
force-pushed
the
support_rewrite_manifests
branch
2 times, most recently
from
5c1ee1f to
dd9c7e6
Compare
| auto new_writer = [this, &schema](const RewriteWriter& rewrite_writer) | ||
| -> Result<std::unique_ptr<ManifestWriter>> { | ||
| std::optional<int64_t> first_row_id = std::nullopt; | ||
| if (base().format_version >= 3 && rewrite_writer.content == ManifestContent::kData) { |
There was a problem hiding this comment.
If the table was upgraded from v2, existing live files may still have null per-file row IDs. Then ManifestEntryAdapterV3::GetFirstRowId falls back to the writer-level value, so those files get 0, and the manifest list skips assignment because the manifest now has a non-null first_row_id. Java leaves the rewrite writer’s manifest firstRowId null via newManifestWriter, allowing manifest-list assignment when needed after upgrade.
There was a problem hiding this comment.
Good catch. I removed the non-null rewrite writer first_row_id logic and added a v2 -> v3 regression test.
| const bool has_direct_replacements = !deleted_manifests_.empty() || | ||
| !added_manifests_.empty() || | ||
| !rewritten_added_manifests_.empty(); | ||
| if (!cluster_by_func_ && !predicate_ && has_direct_replacements) { |
There was a problem hiding this comment.
Java does not rewrite at all when clusterByFunc == null, while this rewrites when predicate_ is set. Is it intentional?
There was a problem hiding this comment.
Not intentional. I changed RequiresRewrite to match Java.
zhjwpku
force-pushed
the
support_rewrite_manifests
branch
from
dd9c7e6 to
3736bc1
Compare
| rewrite_writer.content); | ||
| }; | ||
|
|
||
| std::vector<ManifestFile> result; |
There was a problem hiding this comment.
Closed manifests are kept only in this local result vector until Rewrite() returns, and new_manifests_ is assigned only after the call succeeds in Apply(). If a later length(), WriteExistingEntry, Close(), or
ToManifestFile() returns an error, finalization/cleanup no longer has the already-created manifest paths.
Could we either append closed manifests to member state as soon as they are produced, or clean up the local partial result on the error path? It would also be good to add a test that injects an apply-time manifest writer failure after at least one rolled manifest has been closed.
There was a problem hiding this comment.
Good catch. I'll track rewritten manifests once each writer is closed so that apply-time failures can clean up already-created manifests. Will add a regression test too.
2 participants
No description provided.